auth: wire secure-storage cache into login/token/logout #5013
Draft
simonfaltum wants to merge 11 commits intosimonfaltum/cli-ga-secure-storage-foundationfrom
Draft
auth: wire secure-storage cache into login/token/logout #5013simonfaltum wants to merge 11 commits intosimonfaltum/cli-ga-secure-storage-foundationfrom
simonfaltum wants to merge 11 commits intosimonfaltum/cli-ga-secure-storage-foundationfrom
Conversation
Approval status: pending
|
simonfaltum
changed the title
| // authenticates in the browser, selects a workspace, and the CLI receives | ||
| // the workspace host from the OAuth callback's iss parameter. | ||
| func discoveryLogin(ctx context.Context, dc discoveryClient, profileName string, timeout time.Duration, scopes string, existingProfile *profile.Profile, browserFunc func(string) error) error { | ||
| func discoveryLogin(ctx context.Context, dc discoveryClient, profileName string, timeout time.Duration, scopes string, existingProfile *profile.Profile, browserFunc func(string) error, tokenCache cache.TokenCache) error { |
Contributor
There was a problem hiding this comment.
[optional] it might be time to switch to use a struct as input for readability: https://google.github.io/styleguide/go/best-practices#option-structure
type discoveryLoginInputs struct {
c discoveryClient
profileName string
timeout time.Duration
scopes string
existingProfile *profile.Profile
browserFunc func(string) error
tokenCache cache.TokenCache
}
func discoveryLogin(ctx context.Context, inputs discoveryLoginInputs) error {
// ...
}
Member
Author
There was a problem hiding this comment.
Done, switched to discoveryLoginInputs.
Introduces a thin helper that resolves the CLI's token storage mode via libs/auth/storage.ResolveStorageMode and returns a cache.TokenCache. The helper is split into a public convenience form and an injectable core (newAuthCacheWith) so unit tests exercise the secure path with a fake cache without touching the real OS keyring. Follow-up commits wire this into the auth command call sites.
cache.NewFileTokenCache has a variadic signature (opts ...FileTokenCacheOption), which cannot be assigned directly to a non-variadic field. Adds a one-line explanation so future readers do not 'simplify' the closure back into a direct function reference and break the build.
Adds a hidden --secure-storage BoolVar to auth login and routes the resolved token cache into both NewPersistentAuth call sites (main flow and discoveryLogin). Users opt into OS-native secure storage either per invocation via the flag or for the whole shell via DATABRICKS_AUTH_STORAGE=secure. The default storage mode remains legacy. Flag is MarkHidden because MS1 is experimental; release notes document the env var for discovery.
Resolves the token cache via newAuthCache at the two NewPersistentAuth call sites inside cmd/auth/token.go (loadToken via newTokenCommand and runInlineLogin). No flag is added: per the rollout contract, auth token reads from whichever mode the resolver selects via DATABRICKS_AUTH_STORAGE or [__settings__].auth_storage.
Replaces the direct cache.NewFileTokenCache() call with newAuthCache so that logout targets whichever backend ResolveStorageMode selects. The resolver still defaults to legacy, so existing flows are unchanged; once a user opts into secure storage, logout clears the keyring entry instead of the file cache.
Adds three acceptance tests under acceptance/cmd/auth/storage-modes/: - invalid-env: DATABRICKS_AUTH_STORAGE=bogus surfaces a clear error - legacy-env-default: explicit legacy mode behaves like the default path - hidden-flag: --secure-storage is hidden from auth login --help Secure-mode end-to-end behavior is covered by unit tests in cmd/auth/cache_test.go because acceptance runners cannot safely exercise the real OS keyring on macOS (would write to Keychain) or Linux (no D-Bus session in CI).
Storage backend is a per-machine setting, not a per-invocation choice. Keeping a write-time flag on login while token/logout rely on the resolver creates an asymmetry: login --secure-storage writes to the keyring, but a later auth token without the env var defaults to the legacy file cache and can't find the token. Resolution is now always env -> [__settings__].auth_storage -> default. login.go matches token.go and logout.go: newAuthCache(ctx, "") with no per-call override from a flag. Also drops the hidden-flag acceptance test (no flag to hide) and updates NEXT_CHANGELOG to advertise the env var and config key instead of the flag.
Drop MS1/MS3 comment markers from cache.go and convert discoveryLogin to take a discoveryLoginInputs struct for readability (8 positional args was over the threshold). No behavior change. Co-authored-by: Isaac
simonfaltum
force-pushed
the
simonfaltum/cli-ga-secure-storage-wiring
branch
from
661420f to
1fc0c22
Compare
…e-foundation' into simonfaltum/cli-ga-secure-storage-wiring # Conflicts: # cmd/auth/login.go # cmd/auth/token.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Opt-in OS-native secure token storage for
auth login,auth token, andauth logout. Users turn it on by settingDATABRICKS_AUTH_STORAGE=secure(per-shell) or[__settings__].auth_storage = securein.databrickscfg(per-machine). Everyone else stays on the legacy file-backed cache. The default does not change.Storage backend is a per-machine setting, not a per-invocation choice. This PR deliberately ships no
--secure-storageflag so that login, token, and logout can never drift apart on the same machine.Stacks on #5008. Once that merges, retarget to main.
Changes
Before:
auth login,auth token, andauth logoutalways went through the SDK's default file-backedTokenCache.Now:
cmd/auth/cache.go:newAuthCache(ctx, override)resolves the mode viastorage.ResolveStorageModeand returns the correspondingcache.TokenCache. Split into a public form and an injectable core (newAuthCacheWith) so unit tests exercise the secure path with a fake cache. Production always passesoverride = ""and relies on env -> config -> default.auth login,auth token(vialoadToken+runInlineLogin), andauth logoutall callnewAuthCache(ctx, "")and plumb the resolved cache intou2m.WithTokenCache(...)at everyNewPersistentAuthcall site.acceptance/cmd/auth/storage-modes/cover the two CLI-visible behaviors that don't require an OS keyring: invalid env var surfaces a clear error, and explicit legacy mode behaves identically to the default path.Out of scope:
plaintextstorage implementation. The resolver acceptsplaintext; a follow-up will route it to a non-dual-write file backend.[__settings__].auth_storage. Users hand-edit the config for now.token-cache.jsonentries. Users re-login after upgrading.secure.Test plan
newAuthCachecovering default legacy, explicit override, env-var selection, plaintext fallback, invalid override, invalid env, and file-factory error propagation. Secure path uses a fakecache.TokenCacheso CI never touches the OS keyring.acceptance/cmd/auth/storage-modes/:invalid-env(bogusDATABRICKS_AUTH_STORAGEsurfaces a clear error viaauth token) andlegacy-env-default(explicit legacy mode clears the file cache viaauth logout, matching default behavior).make checks,make test,make lintpass.cmd/auth/login,cmd/auth/token,cmd/auth/logoutacceptance test suites still pass (regression).